Fix #1083: free CJamRecorder if Init() fails - #3838
Merged
Merged
Conversation
pljones
reviewed
Jul 27, 2026
pljones
approved these changes
Jul 27, 2026
Collaborator
|
Looks good apart from the coding style check fail. |
Member
|
Please squash the clang format fix. |
SetRecordingDir() unconditionally news a CJamRecorder, then only wires moveToThread/deleteLater cleanup inside the bRecorderInitialised branch. If Init() fails (e.g. unwritable directory), the freshly-new'd object is orphaned: no thread affinity, no deleteLater connection, no destructor anywhere else to catch it. Free it immediately in the failure branch. The next SetRecordingDir() call overwrites pJamRecorder, so without this fix the leak repeats on every failed recording-dir change.
mcfnord
force-pushed
the
fix-1083-recorder-leak
branch
from
July 29, 2026 02:59
203bb60 to
2cd4ad0
Compare
pljones
reviewed
Jul 29, 2026
| if ( !bRecorderInitialised ) | ||
| { | ||
| delete pJamRecorder; // Init() failed: never moved to thread / never | ||
| pJamRecorder = nullptr; // wired to deleteLater, so free it here (#1083) |
Collaborator
There was a problem hiding this comment.
Slightly dodgy formatting if you want to fix that. LLMs cannot count - particularly spaces.
pljones
approved these changes
Jul 29, 2026
mcfnord
commented
Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.
Fixes the one definitely lost shape from the original valgrind report (the "possibly lost" shapes are the well-known glibc/Qt TLS false-positive pattern and aren't touched here).
The leak:
CJamController::SetRecordingDir()(src/recorder/jamcontroller.cpp) unconditionally doespJamRecorder = new CJamRecorder(...), then callsInit(). ThemoveToThread+deleteLatercleanup wiring only happens inside theif (bRecorderInitialised)branch below it. IfInit()fails — e.g. an unwritable recording directory — the freshly-new'd object has no parent, no thread affinity, and nothing connected todeleteLater.CJamControllerhas no destructor, andCJamRecorder's own constructor takes no parent, so nothing else ever frees it. The next call toSetRecordingDir()overwritespJamRecorder, so the leak repeats on every failed directory change.The fix: delete it immediately in the failure branch, right where
bRecorderInitialisedis set. It's provably safe to delete unconditionally here — this object is only reachable via the localpJamRecorderpointer at this point, never moved to a thread, never connected to anything. (The other branch, further down — a running recorder switching to an empty dir — is a different object already scheduled viadeleteLater; deleting there would double-free, so the fix is scoped only to this one failure path.)Verification: built headless/serveronly locally (
qmake CONFIG+=headless CONFIG+=serveronly && make), clean compile, binary runs. I don't have a way to re-run the original valgrind report here, but the fix is small enough to review by inspection — happy to add a regression test if maintainers want one (e.g. driveSetRecordingDirwith an unwritable path and assert noCJamRecorderoutlives the call).Fixes #1083.